identity/role: GetWalletID swallows storage errors, so a transient DB blip creates a duplicate wallet - #2170

kvs.Exists can't give us the contract this comment promises. It is a bare bool, implemented upstream as len(GetExisting(...)) > 0, and GetExisting swallows store failures:

// fabric-smart-client@v0.17.0/platform/view/services/storage/kvs/kvs.go:110-113
it, err := o.store.GetStateSetIterator(ctx, o.namespace, ids...)
if err != nil {
    return result   // <- error dropped, empty result
}

So on a genuine failure (connection reset, timeout, no such table) we get Exists → false → GetWalletID → ("", nil) → Registry.GetWalletID → WalletIDUnbound, which this PR documents as "a definitive, successful miss: it is safe to fall through … to create a wallet". That is the #2063 failure mode unchanged — and now harder to spot, because the error is dropped inside FSC instead of in our registry. It is a live path, not hypothetical: the integration identity SDK wires the KVS wallet store (integration/token/common/sdk/identity/provider.go:25).

The awkward part is that kvs.Get has no sentinel for absence either — it returns errors.Errorf("state [%s,%s] does not exist", ...) (kvs.go:172-174). Options as I see them, best first:

• Add a kvs.ErrNotFound (or an error-returning Exists/GetIfExists) upstream in FSC and use it here. Cleanest, and other FSC consumers have the same problem.
• Interim: call Get and classify only the "does not exist" error as unbound, propagating everything else. Works today, but matching on an upstream message is fragile — worth a TODO pointing at the FSC issue.
• If neither fits in this PR, then this store cannot satisfy the contract yet, and we should say so explicitly (in the WalletStoreService doc and in identity/role: GetWalletID swallows storage errors, so a transient DB blip creates a duplicate wallet #2063) rather than leave a comment claiming it does.

Either way, a KVS test that makes the underlying store fail and asserts the error propagates would lock this down — right now TestGetWalletIDNotFound only covers real absence.

Failing closed is right at line 131, but I don't think it is here. At this point MapToIdentity has already succeeded: walletID and ident are authoritative, and wID is already in walletIdentifiers. The GetWalletID(passedIdentity) call here is only an optimization — "is this identity already bound to a wallet we can reuse?" — so a failed probe leaves nothing in doubt.

Concretely, after a restart: WalletByID(ctx, role, aliceIdentityBytes) misses the cache, MapToIdentity returns ("alice"), this probe hits a transient DB error → we now return an error and the caller's transaction fails. Before this change we fell through to Role.GetIdentityInfo("alice") and correctly returned/created wallet "alice" — with no duplicate risk, because the id came from the role mapping and WalletByID re-checks the cache under the write lock before creating anything.

Suggest logging at warn and continuing here (and in the same branch at :190), keeping the mapped candidate:

res := r.GetWalletID(ctx, passedIdentity)
if res.Failed() {
    // Optional probe: the mapped wallet id is already authoritative, so a failed
    // lookup only costs us a reuse opportunity, not correctness.
    r.Logger.Warnf("failed to check wallet binding for identity [%s], continuing with mapped wallet [%s]: %v", passedIdentity, wID, res.Err)
} else if res.Bound() {
    ...
}

Line 131 is the one branch where the probe's answer decides whether we create a wallet — that is where fail-closed belongs, and it is correct as written.

Same as the comment on line 166: by here MapToIdentity has succeeded and ident is authoritative, so this probe is an optimization and a storage failure should be logged and skipped rather than aborting the lookup.

Minor, and I think pre-existing: when res is WalletIDUnbound, res.WalletID is "", so this appends an empty id to walletIdentifiers (same at :206). The old code appended the same empty wID, but now that the status is explicit it is easy to guard — if res.Bound() { walletIdentifiers = append(...) }.

The doc says WalletIDUnknown "guards against a WalletIDResolution that was constructed without going through GetWalletID", but nothing actually guards. With only Bound() and Failed() defined, a zero-value WalletIDResolution{} is indistinguishable from WalletIDUnbound at every call site — i.e. the zero value silently means "authoritative miss, safe to create a wallet", which is precisely the interpretation this type was introduced to make impossible. A mock, or a future constructor that forgets to set Status, gets the dangerous default.

Related: the field comment below tells callers to inspect via Bound/Unbound/Failed, but there is no Unbound() method. Adding it and branching exhaustively fixes both:

func (r WalletIDResolution) Unbound() bool { return r.Status == WalletIDUnbound }

…then at each call site treat "neither Bound() nor Unbound()" as a failure.

Registry.GetWalletID changes from (string, error) to WalletIDResolution. Nothing in-tree breaks (the only callers are the three sites in Lookup; wallet.RoleRegistry does not include the method), but Registry is exported API of a released SDK, so downstream code compiled against the old signature will not build.

Either keep GetWalletID(ctx, identity) (string, error) — now returning the honest error instead of swallowing it — and expose the resolution under a new name (ResolveWalletID?), or make sure this lands in the release notes as a breaking change.

require.Equal(t, 0, wf.NewWalletCallCount()) can't fail in these Lookup subtests (same at :195) — Lookup never touches WalletFactory; only WalletByID does (registry.go:371). These assertions would still pass with the old swallow-and-fall-through behaviour restored inside Lookup, so they don't guard the regression. Asserting on the returned error/values is the real check here; TestWalletByID_StorageErrorDoesNotCreateDuplicate is doing the actual work.

Worth adding one more WalletByID case where MapToIdentity succeeds and the probe fails — that is the common path, and the one affected by my comment on registry.go:166.